Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#220 Fix case-sensitivity of HTTP headers in models #364

Open
wants to merge 2 commits into
base: events-v4-serialization-v2
Choose a base branch
from

Conversation

parawanderer
Copy link

Issue #, if available: #220

Description of changes:

This solution builds upon @jeromevdl 's solution of PR 234 to fix the
issue of headers being case sensitive. Fixes @carlzogh 's comment
regarding breaking changes in setHeaders.

This adds some extra unit tests, and reorders some of the added unit test assertions of the original PR, too

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Original PR: #298, I closed that PR by accident due to deleting and re-creating the branch from v4 as a base

@msailes msailes requested review from andclt and smirnoal August 10, 2022 21:02
@richarddd
Copy link
Contributor

richarddd commented Aug 11, 2022

It don't see the value of adding a wrapper around the Tree Map. The only additional feature it has is mergeOrReplace which is always called statically. The factory method could be extracted (if even needed at all) from the class and TreeMap could be used direly

@parawanderer
Copy link
Author

parawanderer commented Aug 12, 2022

@richarddd This PR/code is not solely mine, but in my opinion I see the value in the original author's choosing of a dedicated class to represent HTTP Headers. Observe that that wrapped TreeMap uses a particular case setting parameter (case insensitivity) rather than just being any Map/TreeMap.

You cannot infer this detail from just the fact of using any Map/TreeMap. So configuring this in serialization would be (imo) uglier with either being configured in some sort of config builder outside the class (which makes it harder to understand how the class actually works at a glance), or using some sort of annotation-based specifier on every class member to do with http headers, of which there are a lot. (imo this deals with the "identifying at a glance" issue better but calls for a lot of repetition/low maintainability)

From my perspective as a user of the library, the wrapper allows for some useful documentation. If I need to deep-dive into the library/code, I could see that I'm always dealing with the same special type of map that particularly implements the properties of HTTP Headers.

For the maintainers of the library, maintaining/adding additional properties to this particular Map is centralised, and would probably integrate more easily into any typical serialization framework. E.g. changing the internal implementation to maybe use a regular hashmap with case-insensitivity built in in some other way is also easier.

I see the argument of there being a bunch of generic code/duplication, and could see a perspective of just extending TreeMap and providing that constructor parameter to the parent constructor. But in that case you could end up with people using the map as a TreeMap potentially expecting the properties of a TreeMap, when the fact that it is such is coincidence and could be changed at a later time. So in my opinion it's still an useful level of abstraction that helps maintainability there, too.

WDYT?

EDIT: I got completely sidetracked and forgot about using a factory. I think this is part of that "configuration being hard to follow/maintain" issue too, though. (Unless there's a way to do that to sidestep both issues, of course)

@richarddd
Copy link
Contributor

WDYT?

Ah right, i did not think about serializing/deserializing makes sense to have then as a wrapper class

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants